Skip to content

fix!: replace custom glob with minimatch for improved pattern matching #750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main-enterprise
Choose a base branch
from

Conversation

PendaGTP
Copy link
Contributor

@PendaGTP PendaGTP commented Jan 24, 2025

Description

Fixes #748

Replace custom glob implementation with minimatch.

Changes

  • Replace custom glob implementation with minimatch to fully support glob patterns
  • Remove support for Regex patterns
  • Simplify logic for matching glob patterns
  • Update and improve related documentation

⚠️ BREAKING CHANGES: ⚠️

Removed Regex support for patterns. Users must update their configuration to use glob syntax.

Before:

exclude: ['^admin$', '^\.github$', '^safe-settings$', '.*-test']

After:

restrictedRepos: ['admin', '.github', 'safe-settings', '*-test']

This change simplifies the configuration and doesn't significantly impact existing functionality.

Testing

  • Ensured all unit tests pass
  • Removed glob.test.ts as it's no longer necessary
  • Performed minimal manual testing

@PendaGTP PendaGTP force-pushed the fix/glob-pattern-matching branch 2 times, most recently from 9232961 to b3aba96 Compare January 24, 2025 18:37
@PendaGTP PendaGTP changed the title fix: replace custom glob with minimatch for improved pattern matching fix!: replace custom glob with minimatch for improved pattern matching Jan 24, 2025
@PendaGTP PendaGTP force-pushed the fix/glob-pattern-matching branch from b3aba96 to a247e01 Compare January 24, 2025 18:46
@PendaGTP
Copy link
Contributor Author

Note:

This PR doesn't impact:

  • labels regex
  • validator regex

@decyjphr
Copy link
Collaborator

This is great! But requires some manual resolution of conflicts. So will merge it after handling a few simpler PR merges.

@PendaGTP
Copy link
Contributor Author

Let me know if (and when) you need help to resolve conflicts.

@decyjphr
Copy link
Collaborator

Thanks @PendaGTP if you could look at that it would be awesome. It is mostly the code you have contibuted in the previous PRs and this, so it would be easier for you.

@PendaGTP PendaGTP force-pushed the fix/glob-pattern-matching branch from a247e01 to 99509e8 Compare March 22, 2025 14:18
@PendaGTP
Copy link
Contributor Author

@decyjphr fyi rebase done

@bmike78
Copy link

bmike78 commented Apr 17, 2025

I do want to comment that this fix by @PendaGTP works much better than the recent #808 merge when it comes to mistaken globbing issues.

Basically as it stands with current main-enterprise branch with #808, if there is a config like this:

suborgrepos:
  - luv

Then it will be greedy and pull in any repos that match "luv" in any part of the repo name, whereas in this PR, the behavior is that this config would only match the repo called "luv" and that's all.

Testing

  • Tested globbing works, for example:
suborgrepos:
  - luv*

This will match any repo that starts with "luv", for example: "luv-mike-repo" will match.

  • This did not seem to work:
suborgrepos:
  - *luv*

Our team is good with this behavior, just pointing out that we also tested this scenario.

Background

  • About 7-8 months ago, we had a subort config with "admin" in the name that caused safe settings to deploy on several dozen repos that matched "admin" in any part of the repo name.
  • Due to this, we diverged from the standard glob.js, but it looks like this PR will allow us to join back up with upstream safe settings once this PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suborgproperties is matching too many repositories
3 participants